Skip to content

tests: added test for Body() methods of requests using golden files #450

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bigbes
Copy link

@bigbes bigbes commented Jul 3, 2025

This pull request introduces changes to the test data files for various request types.

Base automatically changed from bigbes/TNTP-3334-iproto-is-sync-support to master July 3, 2025 10:11
@bigbes bigbes force-pushed the bigbes/TNTP-3334-iproto-body-tests branch 2 times, most recently from 5977e48 to 8897eb5 Compare July 7, 2025 13:53
@bigbes bigbes requested a review from oleg-jukovec July 7, 2025 13:54
@bigbes bigbes force-pushed the bigbes/TNTP-3334-iproto-body-tests branch 2 times, most recently from 55de81c to fe36442 Compare July 7, 2025 14:52
@bigbes bigbes requested a review from dmyger July 7, 2025 15:42
@bigbes bigbes marked this pull request as ready for review July 7, 2025 17:19
// ```
// Use it to debug the test.
//
// If you want to update the golden file, run delete old file and rerun the test.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to you, but I think it would be more reasonable to explicitly create data files with the update-testdata flag or something similar.
Otherwise, the absence of a control file will result in the test passing successfully.
As for an example, you could check: https://github.com/tarantool/tt/blob/master/cli/tcm/log_test.go#L23

Copy link
Collaborator

@oleg-jukovec oleg-jukovec Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Ideally we need to separate generation of data from tests and generate it with go generate. But a flag a good idea too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

golden_test.go Outdated
BitwiseOr(8, 11).
BitwiseXor(9, 12)

for _, tc := range []goldenTestCase{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better to separate the initialisation of test cases and the logic of the test loop.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you please verify if this change matches what you had in mind?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant a more classic solution with a separate variable for cases and a simple loop, without creating and initialising a complex multi-line structure:

Suggested change
for _, tc := range []goldenTestCase{
testCases := []goldenTestCase{
...
}
for _, tc := range testCases{

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done!

//
// If you want to update the golden file, run delete old file and rerun the test.

func logMsgpackAsJsonConvert(t *testing.T, data []byte) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func logMsgpackAsJsonConvert(t *testing.T, data []byte) {
func logMsgpackAsJsonConvert(t *testing.T, data []byte) {
t.Helper()

Helper function?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed!

Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the patch. See comments above.

@bigbes bigbes force-pushed the bigbes/TNTP-3334-iproto-body-tests branch from fe36442 to ca0c7f6 Compare July 9, 2025 10:43
err := req.Body(&dummySchemaResolver{}, encoder)
require.NoError(t, err, "failed to encode request")

goldenFileExists := fileExists(name)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand why it is necessary to check for the existence of a file here.

  1. If we generate new ones, we simply overwrite them, regardless of whether the old ones exist. But it's still better to automatically delete all the old ones before running the tests so that no rubbish is saved if there have been any changes to the tests.
  2. If there is no file for the test, there will be an error when trying to read it anyway.

Copy link
Author

@bigbes bigbes Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something that I want to avoid:

  • The Tarantool protocol is unlikely to change, so the behavior of those tests won't need frequent updates. New tests will be added, but existing tests should mostly remain unchanged.
  • If a test is modified, I don’t want to require rewriting all tests — this could eventually introduce bugs due to human error (e.g., accidentally committing test data without proper review). Instead, I want the user to remove only the test cases affected by their change, after carefully considering potential mistakes.

@bigbes bigbes force-pushed the bigbes/TNTP-3334-iproto-body-tests branch from ca0c7f6 to babd1e3 Compare July 9, 2025 13:27
@oleg-jukovec oleg-jukovec requested a review from dmyger July 10, 2025 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants